Reuse known fileids and cache data in the scanner #10993

icewind1991 · 2014-09-10T14:15:04Z

Saves having to do multiple getId calls for every file and folder being scanned.

Saves about ~~50%~~ ~~80%~~ 90% of the database class made during a filescan

cc @PVince81 @DeepDiver1975 @th3fallen

icewind1991 · 2014-09-10T14:55:16Z

Found an even bigger improvement, now the cache data itself is being reused, bringing the optimal case (cache is up to date) to 3 queries per folder, down from 5 per folder and 3 per file.

In my local environment this brings scanning the ~44k files down to ~32k queries and 190sec compared to the original 160k queries and 374s

icewind1991 · 2014-09-10T22:50:29Z

Another round of optimization later and we're down to 1 query per folder in the best case, resulting in a total of 14k in my test cases vs 160k before optimazation

PVince81 · 2014-09-11T10:20:43Z

It sounds like a cool improvement, but I'm worried that we will run into situations like #10954 (comment) more often if we cache too much.

Or we need a way to auto-clean older cache entries while the scanning is in progress.

PVince81 · 2014-09-11T10:22:03Z

If you have the folder structure:

/a
/a/subdir
/b

As soon as scanning of /a i finished, I guess it's an opportunity to delete everything in the cache related to /a ?

icewind1991 · 2014-09-11T11:45:00Z

This actually uses (slightly) less memory as before, this re-uses objects which were already kept in memory instead of requesting another copy of the object from the database

icewind1991 · 2014-09-23T12:30:42Z

@DeepDiver1975 @PVince81 @th3fallen can this be merged?

DeepDiver1975 · 2014-09-23T12:43:06Z

lib/private/files/cache/scanner.php

@@ -104,9 +104,11 @@ public function getData($path) {
 	 *
 	 * @param string $file
 	 * @param int $reuseExisting
+	 * @param int $parentId
+	 * @param array |null $cacheData


cacheData of the parent or the file to be scanned?

Added documentation

Mhm - I don't see this clarified yet.

Weird, added it now

PVince81 · 2014-09-23T13:35:28Z

Maybe after the release, when there will be more time to review OC 8 stuff...

DeepDiver1975 · 2014-10-13T11:28:59Z

fixes #8548

PVince81 · 2014-10-31T13:27:13Z

lib/private/files/cache/scanner.php

+			$folderId = $this->cache->getId($path);
+		}
+		$existingChildren = $this->getExistingChildren($folderId);
+		$newChildren = $this->getNewChildren($path);


Can you explain this addition ? Does it mean we previously rescanned all children, not only the new ones ?

$newChildren here means the children as reported by the storage, as opposed to the children reported by the cache ($existingChildren)

PVince81 · 2014-10-31T13:29:56Z

The code looks good but I don't understand it 100%.

LukasReschke · 2014-10-31T13:32:10Z

lib/private/files/cache/cache.php

@@ -194,6 +194,9 @@ public function getFolderContentsById($fileId) {
 					$file['size'] = $file['unencrypted_size'];
 				}
 				$file['permissions'] = (int)$file['permissions'];
+				$file['mtime'] = (int)$file['mtime'];
+				$file['storage_mtime'] = (int)$file['storage_mtime'];
+				$file['size'] = (int)$file['size'];


Will this lead to problems on 32bit platforms?

@icewind1991 what about this comment ?

MorrisJobke · 2014-12-19T08:45:16Z

@icewind1991 Want to rebase this and made it ready for review or is this something for after 8.0?

icewind1991 · 2015-02-02T17:24:15Z

comparison

DeepDiver1975 · 2015-02-02T18:55:54Z

Test Name   Duration    Age
 Test_Share.testShareWithUserAndUserIsExcludedFromResharing
 Stack Trace

Test_Share::testShareWithUserAndUserIsExcludedFromResharing
Failed asserting that user 4 is excluded from re-sharing
Failed asserting that 31 is identical to 15.

/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/tests/lib/share/share.php:451

    1.440918    1
 Test_Util::testIsSharingDisabledForUser.testIsSharingDisabledForUser with data set #5
 Stack Trace

Test_Util::testIsSharingDisabledForUser with data set #5 (array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1', 'g2'), true)
Failed asserting that false matches expected true.

/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/tests/lib/util.php:305

    0.056838    1
 Test_Util::testIsSharingDisabledForUser.testIsSharingDisabledForUser with data set #6
 Stack Trace

Test_Util::testIsSharingDisabledForUser with data set #6 (array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1', 'g2', 'g3'), true)
Exception: The username is already being used

/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/user/manager.php:256
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/user.php:186
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/tests/lib/util.php:289

    0.012676    1
 OCA\Files_Encryption\Tests\Webdav.testWebdavPUT
 Stack Trace

OCA\Files_Encryption\Tests\Webdav::testWebdavPUT
Undefined index: fileid

/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/files/cache/scanner.php:146
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/files/cache/scanner.php:235
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/files/cache/updater.php:54
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/files/view.php:536
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/connector/sabre/file.php:137
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/lib/private/connector/sabre/directory.php:84
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/3rdparty/sabre/dav/lib/Sabre/DAV/Server.php:1647
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/3rdparty/sabre/dav/lib/Sabre/DAV/Server.php:900
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/3rdparty/sabre/dav/lib/Sabre/DAV/Server.php:474
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/3rdparty/sabre/dav/lib/Sabre/DAV/Server.php:214
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/apps/files_encryption/tests/webdav.php:257
/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/apps/files_encryption/tests/webdav.php:117

    2.032317    1
 Test_Files_Sharing_Api.testSharePermissions
 Stack Trace

Test_Files_Sharing_Api::testSharePermissions
Failed asserting that true is false.

/var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/apps/files_sharing/tests/api.php:243

PVince81 · 2015-02-09T10:53:24Z

Please rebase + squash and fix the unit tests, if applicable 😄

icewind1991 · 2015-02-10T16:53:22Z

@DeepDiver1975 @PVince81 @MorrisJobke merge?

PVince81 · 2015-02-11T09:23:52Z

icewind1991 · 2015-02-11T12:12:23Z

squashed and fixed the 32bit int issue

scrutinizer-notifier · 2015-02-11T12:26:06Z

The inspection completed: 5 new issues, 2 updated code elements

ghost · 2015-02-11T12:47:47Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9196/
Test PASSed.

PVince81 · 2015-02-11T14:10:22Z

Tested against SFTP ext storage with remote changes, scanning still works correctly 👍

MorrisJobke · 2015-02-12T09:52:57Z

Tested 👍

Reuse known fileids and cache data in the scanner

PVince81 · 2015-02-12T17:50:07Z

Regression found: #14169

icewind1991 changed the title ~~Reuse known fileids in the scanner~~ Reuse known fileids and cache data in the scanner Sep 10, 2014

LukasReschke added the 3 - To Review label Sep 10, 2014

icewind1991 force-pushed the scanner-reuse-fileid branch from 3883d96 to 5724c12 Compare September 10, 2014 22:49

PVince81 added enhancement performance labels Sep 11, 2014

DeepDiver1975 force-pushed the scanner-reuse-fileid branch from 5724c12 to ea69cd1 Compare September 11, 2014 10:19

DeepDiver1975 reviewed Sep 23, 2014
View reviewed changes

icewind1991 force-pushed the scanner-reuse-fileid branch from 7caf1c5 to 64e26d3 Compare September 24, 2014 13:28

icewind1991 mentioned this pull request Sep 24, 2014

Don't keep the full info of all children in memory #11282

Merged

scolebrook force-pushed the master branch from 4a4ea1d to 9dea79e Compare October 10, 2014 13:32

icewind1991 force-pushed the scanner-reuse-fileid branch 3 times, most recently from 75be54a to 268c5cb Compare October 17, 2014 11:39

PVince81 reviewed Oct 31, 2014
View reviewed changes

LukasReschke reviewed Oct 31, 2014
View reviewed changes

PVince81 mentioned this pull request Nov 28, 2014

Storage changes overview #12487

Closed

22 tasks

MorrisJobke added 2 - Developing and removed 3 - To Review labels Dec 19, 2014

MorrisJobke added 1 - To develop and removed 2 - Developing labels Dec 19, 2014

icewind1991 force-pushed the scanner-reuse-fileid branch from c3d950f to 5862e4f Compare February 2, 2015 17:23

PVince81 added this to the 8.1-next milestone Feb 2, 2015

icewind1991 force-pushed the scanner-reuse-fileid branch 2 times, most recently from 3c1b6e6 to ddcd9ce Compare February 10, 2015 15:06

icewind1991 added 4 commits February 11, 2015 13:08

Reuse cache data of existing files during scan

4242dd0

Cast mtimes and size to int

dc6468c

Only try to scan the children of directories

9bbfead

only read permissions once

9df18ff

icewind1991 force-pushed the scanner-reuse-fileid branch from b31ad2b to 9df18ff Compare February 11, 2015 12:12

MorrisJobke added a commit that referenced this pull request Feb 12, 2015

Merge pull request #10993 from owncloud/scanner-reuse-fileid

f4182d2

Reuse known fileids and cache data in the scanner

MorrisJobke merged commit f4182d2 into master Feb 12, 2015

MorrisJobke deleted the scanner-reuse-fileid branch February 12, 2015 09:53

MorrisJobke mentioned this pull request Feb 12, 2015

Reuse parent fle info on recursive file scan #8548

Closed

MorrisJobke added 3 - To Review and removed 1 - To develop labels Feb 12, 2015

PVince81 mentioned this pull request Feb 12, 2015

Cannot WebDAV upload file into shared folder as recipient with enc enabled #14169

Closed

PVince81 mentioned this pull request Jun 1, 2015

scanFile() does not return file id if scanned file matches cache entry 100% (same mtime) #16659

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse known fileids and cache data in the scanner #10993

Reuse known fileids and cache data in the scanner #10993

icewind1991 commented Sep 10, 2014

icewind1991 commented Sep 10, 2014

icewind1991 commented Sep 10, 2014

PVince81 commented Sep 11, 2014

PVince81 commented Sep 11, 2014

icewind1991 commented Sep 11, 2014

icewind1991 commented Sep 23, 2014

DeepDiver1975 Sep 23, 2014

icewind1991 Sep 23, 2014

LukasReschke Oct 13, 2014

icewind1991 Oct 15, 2014

PVince81 commented Sep 23, 2014

DeepDiver1975 commented Oct 13, 2014

PVince81 Oct 31, 2014

icewind1991 Oct 31, 2014

PVince81 commented Oct 31, 2014

LukasReschke Oct 31, 2014

PVince81 Feb 11, 2015

MorrisJobke commented Dec 19, 2014

icewind1991 commented Feb 2, 2015

DeepDiver1975 commented Feb 2, 2015

PVince81 commented Feb 9, 2015

icewind1991 commented Feb 10, 2015

PVince81 commented Feb 11, 2015

icewind1991 commented Feb 11, 2015

scrutinizer-notifier commented Feb 11, 2015

ghost commented Feb 11, 2015

PVince81 commented Feb 11, 2015

MorrisJobke commented Feb 12, 2015

PVince81 commented Feb 12, 2015

Reuse known fileids and cache data in the scanner #10993

Reuse known fileids and cache data in the scanner #10993

Conversation

icewind1991 commented Sep 10, 2014

icewind1991 commented Sep 10, 2014

icewind1991 commented Sep 10, 2014

PVince81 commented Sep 11, 2014

PVince81 commented Sep 11, 2014

icewind1991 commented Sep 11, 2014

icewind1991 commented Sep 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Sep 23, 2014

DeepDiver1975 commented Oct 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Oct 31, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Dec 19, 2014

icewind1991 commented Feb 2, 2015

DeepDiver1975 commented Feb 2, 2015

PVince81 commented Feb 9, 2015

icewind1991 commented Feb 10, 2015

PVince81 commented Feb 11, 2015

icewind1991 commented Feb 11, 2015

scrutinizer-notifier commented Feb 11, 2015

ghost commented Feb 11, 2015

PVince81 commented Feb 11, 2015

MorrisJobke commented Feb 12, 2015

PVince81 commented Feb 12, 2015